AMDGPU: add hotswap entry trampoline core by harsh-amd · Pull Request #3008 · ROCm/llvm-project

harsh-amd · 2026-06-22T12:51:59Z

Stack:

This PR - core hotswap entry trampoline implementation, explicit COMGR options API, unit coverage, and minimal public LIT coverage.
AMDGPU: add hotswap entry trampoline lit coverage #3098 - expanded LIT coverage layered on this PR.

The temporary ROCm base branch users/harsh/comgr-hotswap-entry-core mirrors this PR head so #3098 shows only the incremental LIT diff.

Summary:

Adds opt-in gfx125x hotswap entry trampoline generation and descriptor fixups.
Exposes amd_comgr_hotswap_rewrite_with_options and AMD_COMGR_HOTSWAP_REWRITE_FLAG_ENTRY_TRAMPOLINES so callers request entry trampolines explicitly per rewrite call.
Removes COMGR production dependence on AMD_COMGR_HOTSWAP_ENTRY_TRAMPOLINES; COMGR no longer uses that env var as the integration contract.
Keeps the existing amd_comgr_hotswap_rewrite ABI-compatible default path without entry trampolines.
Wires the rewrite pipeline to fail closed when requested entry trampolines cannot be installed.
Tightens entry-stub idempotency by validating decoded operand shape and that the stub targets earlier .text.
Aligns appended entry stubs by virtual address and preserves the instruction-prefetch guard.
Makes program-header growth overflow handling fail closed instead of skipping malformed headers.
Adds ELF helpers for enumerating/updating kernel descriptors and SGPR metadata.
Adds focused unit coverage for the ELF helpers, trampoline builder, descriptor rewrites, SGPR metadata updates, idempotency, operand validation, alignment, and failure paths.
Updates the shared hotswap-rewrite LIT driver to exercise amd_comgr_hotswap_rewrite_with_options.
Adds minimal public LIT coverage for default-off behavior, explicit options-API opt-in entry trampolines, invalid options, and fail-closed scratch-SGPR exhaustion.

Verification:

Branch head: 80cbb28.
Stack top head: f80a756.
python3 amd/comgr/utils/check_api_consistency.py --comgr-dir amd/comgr passed.
cmake --build build --target hotswap-rewrite -- -j8 passed.
build/bin/llvm-lit -sv build/tools/comgr/test-lit --filter 'hotswap-rewrite.c|hotswap-kernel-entry-trampoline.s' passed: 2 tests on AMDGPU: add hotswap entry trampoline core #3008.
build/bin/llvm-lit -sv build/tools/comgr/test-lit --filter hotswap passed: 53 tests on the stack top.
ASAN cmake --build build-asan --target hotswap-rewrite -- -j8 passed.
ASAN build-asan/bin/llvm-lit -sv build-asan/tools/comgr/test-lit --filter hotswap passed: 53 tests on the stack top.
Earlier full normal check-comgr passed after the options API update: lit 72 passed/11 unsupported; unit tests passed; CTest 31/31 passed.
Earlier full ASAN check-comgr passed after the options API update: lit 72 passed/11 unsupported; unit tests passed; CTest 31/31 passed.

chinmaydd · 2026-06-30T01:33:05Z

+  const llvm::StringRef Expected[] = {"v_mbcnt_lo_u32_b32",
+                                      "v_mbcnt_hi_u32_b32",
+                                      "v_lshlrev_b32",
+                                      "v_add_nc_u32",
+                                      "v_and_b32",
+                                      "ds_store_b32",
+                                      "s_branch"};


This is an improvement, but it feels rather incomplete if we dont check the operands of the rewritten instructions.

We should definitely have integration tests, but IMO it should also be fixed in Unit tests.

Same for others...

Fixed. Unit tests now compare decoded MCInst operands against expected assembled instructions for ADDTID load/store bodies and the entry trampoline.

chinmaydd · 2026-06-30T01:38:17Z

+// RUN: %clang -target amdgcn-amd-amdhsa -mcpu=gfx1250 -nostdlib %s -o %t.elf
+
+// RUN: AMD_COMGR_HOTSWAP_ENTRY_TRAMPOLINES=1 hotswap-rewrite %t.elf \
+// RUN:   amdgcn-amd-amdhsa--gfx1250 amdgcn-amd-amdhsa--gfx1250 \


This style of the triple is going to be deprecated soon. But I suppose we will take of it after that change flows in Comgr from upstream...

This style of the triple is going to be deprecated soon.
This is news to me. What is it going to be moving forward?

But I suppose we will take of it after that change flows in Comgr from upstream...
Thank you :)

You can refer to the PR stack here to get a clue : https://github.com/llvm/llvm-project/pull/206488/changes.

I am trying to get a hold of the presentation that Matt gave to the compiler team yesterday. I'll share it with others when I have it.

Effectively, moving towards:

amdgpu12.50-amd-amdhsa

chinmaydd · 2026-06-30T02:23:53Z

I have read through the code generally, but admittedly, there's a lot of it. I'm sure to have missed things. I ran it through Claude and here are some issues that we should probably take a look at:

patchDebugSections gets Deferred but the ELF grew by Growth (comgr-hotswap-b0a0.cpp:636): symbols/line-rows cover only B0→A0 trampolines while the address shift includes entry-stub bytes; in the entry-only path zero symbols are added for a non-zero shift (latent: patchDebug* are weak no-ops today).
isKernelEntryTrampoline matches opcodes only, not the jump target (comgr-hotswap-entry-trampoline.cpp:144/318): a real kernel beginning with the same 6-instruction prologue is misread as "already stubbed" and silently skipped.
Stub 256-byte alignment computed on file offset, not vaddr (comgr-hotswap-entry-trampoline.cpp:343): textSize-relative rounding misaligns the redirected entry whenever textAddr % 256 != 0.
getKernelSgprCount drops the KD fallback for malformed/partial metadata (comgr-hotswap-elf.cpp:691): any present-but-bad AMDGPU note now hard-returns nullopt, aborting the rewrite where the old descriptor fallback succeeded.
updateKernelDescriptorSgprCount writes RSRC1 from an 8 baseline while metadata is the true source (comgr-hotswap-elf.cpp:648): leaves descriptor/metadata SGPR counts divergent on GFX12 and can over-reserve up to a 112-SGPR granule.
adjustProgramHeaders swallows overflow with continue instead of return false (comgr-hotswap-elf.cpp:982): a segment can be left un-grown, yielding a "successful" rewrite whose PT_LOAD no longer spans the enlarged .text.
adjustSymbolValues relocates symbols by file-offset order, not vaddr (comgr-hotswap-elf.cpp:1135): a section whose file order differs from vaddr order gets its address shifted but its symbols' st_value left stale.
Duplicate base-name .kd symbols resolve inconsistently across the two ElfViews (comgr-hotswap-entry-trampoline.cpp): one descriptor can be rewritten twice (last-writer-wins) while the other keeps an orphaned stub.
descriptorAlreadyTargetsEntryStub treats an entryVAddr() failure as "not a stub" (comgr-hotswap-entry-trampoline.cpp:318): a descriptor at the overflow boundary is re-processed and gets a second stub, defeating idempotency.

AFAICT, these are really corner cases that we probably wont encounter in well-formed kernels. In that sense, I think we should be okay.

harsh-amd · 2026-06-30T06:45:52Z

I have read through the code generally, but admittedly, there's a lot of it. I'm sure to have missed things. I ran it through Claude and here are some issues that we should probably take a look at:

patchDebugSections gets Deferred but the ELF grew by Growth (comgr-hotswap-b0a0.cpp:636): symbols/line-rows cover only B0→A0 trampolines while the address shift includes entry-stub bytes; in the entry-only path zero symbols are added for a non-zero shift (latent: patchDebug* are weak no-ops today).

isKernelEntryTrampoline matches opcodes only, not the jump target (comgr-hotswap-entry-trampoline.cpp:144/318): a real kernel beginning with the same 6-instruction prologue is misread as "already stubbed" and silently skipped.

Stub 256-byte alignment computed on file offset, not vaddr (comgr-hotswap-entry-trampoline.cpp:343): textSize-relative rounding misaligns the redirected entry whenever textAddr % 256 != 0.

getKernelSgprCount drops the KD fallback for malformed/partial metadata (comgr-hotswap-elf.cpp:691): any present-but-bad AMDGPU note now hard-returns nullopt, aborting the rewrite where the old descriptor fallback succeeded.

updateKernelDescriptorSgprCount writes RSRC1 from an 8 baseline while metadata is the true source (comgr-hotswap-elf.cpp:648): leaves descriptor/metadata SGPR counts divergent on GFX12 and can over-reserve up to a 112-SGPR granule.

adjustProgramHeaders swallows overflow with continue instead of return false (comgr-hotswap-elf.cpp:982): a segment can be left un-grown, yielding a "successful" rewrite whose PT_LOAD no longer spans the enlarged .text.

adjustSymbolValues relocates symbols by file-offset order, not vaddr (comgr-hotswap-elf.cpp:1135): a section whose file order differs from vaddr order gets its address shifted but its symbols' st_value left stale.

Duplicate base-name .kd symbols resolve inconsistently across the two ElfViews (comgr-hotswap-entry-trampoline.cpp): one descriptor can be rewritten twice (last-writer-wins) while the other keeps an orphaned stub.

descriptorAlreadyTargetsEntryStub treats an entryVAddr() failure as "not a stub" (comgr-hotswap-entry-trampoline.cpp:318): a descriptor at the overflow boundary is re-processed and gets a second stub, defeating idempotency.

AFAICT, these are really corner cases that we probably wont encounter in well-formed kernels. In that sense, I think we should be okay.

Thanks. I fixed the entry-trampoline issues from this list: opcode-only idempotency, vaddr alignment, program-header overflow, entryVAddr idempotency failure, and missing operand checks in unit tests.

I left the debug/metadata/symbol-order/duplicate-name items unchanged since they are broader ELF/debug/metadata policy issues outside this PR’s entry-trampoline path.

harsh-amd · 2026-06-30T14:28:55Z

Also I modified this PR so comgr no longer depends on the global environment var. The idea is that rocr runtime will call comgr with the appropriate rewrite options depending on the value of that global environment var. The existing amd_comgr_hotswap_rewrite path remains ABI-compatible and keeps entry trampolines off by default, while callers like rocm-systems can opt in only for the gfx12.5 cases they own using amd_comgr_hotswap_rewrite_with_options.

chinmaydd · 2026-06-30T14:49:17Z

Also I modified this PR so comgr no longer depends on the global environment var. The idea is that rocr runtime will call comgr with the appropriate rewrite options depending on the value of that global environment var

I see, thanks. Yes, I think thats the right thing to do.

However, the LIT test (and its driver) does not exercise the new path yet.

harsh-amd · 2026-06-30T16:06:47Z

Done. I moved the minimal public LIT coverage into this PR so #3008 now exercises the new options API directly. The shared hotswap-rewrite driver has --entry-trampolines and invalid-options modes, and hotswap-kernel-entry-trampoline.s now uses amd_comgr_hotswap_rewrite_with_options through that driver instead of the removed env var path. #3098 was rebased so it only carries the expanded LIT matrix.

Verified normal and ASAN hotswap lit: 53 tests passed in each build.

chinmaydd · 2026-06-30T18:24:10Z

+  if (!Options.RunB0A0Patches && !Options.RunEntryTrampolines) {
+    std::unique_ptr<WritableMemoryBuffer> Result =
+        WritableMemoryBuffer::getNewUninitMemBuffer(ElfSize);
+    if (!Result) {
+      log() << "hotswap: error: retargetCodeObject: "
+            << "getNewUninitMemBuffer(" << ElfSize
+            << ") failed (out of memory) for the no-op output copy.\n";
+      return AMD_COMGR_STATUS_ERROR_OUT_OF_RESOURCES;
+    }
+    std::memcpy(Result->getBufferStart(), ElfData, ElfSize);
+    Out = std::move(Result);
+    return AMD_COMGR_STATUS_SUCCESS;
+  }


I think we're almost there.

This no-op behavior should be documented somewhere

Its also weird that there's no way for the caller to know whether this was exercised.

I agree this is not ideal, but I would prefer not to grow the API shape in this PR. For now, callers that need to know can compare input/output bytes. If ROCr needs structured pass-result reporting, we should add that as a follow-up API extension.

Done. I documented this in the public hotswap API comments and the HotSwap README. SUCCESS means comgr produced a valid output code object, not that bytes necessarily changed; if source/target/options select no enabled rewrite, output is a copy of the input.

chinmaydd · 2026-06-30T18:26:56Z

-/// amd_comgr_hotswap_rewrite, and optionally dumps the output and/or checks
-/// that a second rewrite produces identical output (idempotency).
+/// the hotswap rewrite API, and optionally dumps the output and/or checks that
+/// a second rewrite produces identical output (idempotency).


Do we need to claim idempotency? This is a strong claim and I wonder if we have tests for it.

When B0A0 and entry trampolines run together, the B0-to-A0 pass re-decodes the whole grown .text (now including the appended stub pool) and has no 'skip existing stub' guard, unlike the entry pass (Work-empty check).

Agreed. I reworded the driver comment. The intent is only that the test driver can rerun the same request and compare outputs, not to claim global idempotency for every combination of rewrite options.

chinmaydd · 2026-06-30T18:31:04Z

There's also a bunch of redundancy in implementation, but we can take that up in the refactor.

harsh-amd · 2026-06-30T18:51:26Z

Sounds good. I will leave the broader cleanup for the follow-up refactor and keep this PR focused on the API and entry-trampoline behavior.

chinmaydd · 2026-06-30T19:54:53Z

Thanks. This mostly LGTM.

@lamb-j is planning to look as well, so I'd recommend we wait for his inputs.

harsh-amd · 2026-06-30T22:03:05Z

Thanks. This mostly LGTM.

@lamb-j is planning to look as well, so I'd recommend we wait for his inputs.

Sounds good. Can we land this and address his comments in the follow up PR?

chinmaydd · 2026-06-30T22:19:23Z

Sure. When I had a chat with him, I think a concern was about adding a new Comgr API without incrementing the semantic versioning. Although, in this case I dont think that would be an issue.

Its okay to land for now.

lamb-j · 2026-06-30T22:48:42Z

Filed #3134

Ideally in the future, we do the version bump in the same commit as the API introduction. It's easier to cherry-pick, revert, etc. if they're together.

Also if we're really unlucky, a branch can be cut between the API introduction and the version bump, which exposes us to some pretty big inconsistencies. Not as big a deal for minor bumps compared to major, but still not ideal

## Summary Adds an explicit API-versioning rule to `amd/comgr/AGENT_CONVENTIONS.md` (section 4, PR workflow): every commit that adds a public `AMD_COMGR_API` function must bump the minor version in `VERSION.txt` and tag the new prototype with a fresh `AMD_COMGR_VERSION_X_Y` macro. This codifies the policy that surfaced when #3008 added `amd_comgr_hotswap_rewrite_with_options` under the already-used `3.3` version without bumping (fixed in its follow-up #3134). The rule captures: - One bump per commit, not per function. - Never reuse the current version's macro for a new API in a later commit. - Don't wait for an official release to bump — we don't know which commit lands in each release, so the version advances when the API is introduced. - Add the symbol to `exportmap.in` under a version node matching the new tag. - `check_api_consistency.py` only enforces `VERSION.txt >=` the highest macro, so it cannot catch version reuse — verify the bump by hand. ## Changes - `AGENT_CONVENTIONS.md`: add the "Bump the version when adding a public API" subsection.

#3134) ## Summary Follow-up to #3008, which added the `amd_comgr_hotswap_rewrite_with_options` API but tagged it `AMD_COMGR_VERSION_3_3` — a version already consumed by the block-size kernel APIs in #1946. Per our policy, every commit that adds a new public API must also bump the minor version, so the hotswap options API should own its own version. The `check_api_consistency.py` script only enforces `VERSION.txt >=` the highest tag macro, so the duplicate `3.3` tag passed CI even though it reused an existing version. ## Changes - `VERSION.txt`: bump minor `3` -> `4`. - `amd_comgr.h.in`: add the `AMD_COMGR_VERSION_3_4` macro and retag `amd_comgr_hotswap_rewrite_with_options` from `3.3` to `3.4`. - `exportmap.in`: move `amd_comgr_hotswap_rewrite_with_options` into a new `@amd_comgr_NAME@_3.4` symbol-version node inheriting from `_3.3`. The symbol-version change is safe: `3.3` has not shipped in a release, so no external consumer depends on the `@_3.3` versioned symbol. `python3 amd/comgr/utils/check_api_consistency.py --comgr-dir amd/comgr` passes (`VERSION.txt=3.4`, 69 APIs declared and exported).

## JIRA ID JIRA ID - ROCM-27304 ## Stack Layer 2 of the rocm-systems HotSwap stack, paired with ROCm/llvm-project#3008. - Previous layer: #7577 - This layer: rocm-systems loader gating and COMGR request selection for the gfx12.5 entry-trampoline path - Final layer: #7592 ## Summary - Keep rocm-systems limited to loader routing and COMGR source/target ISA-pair construction; COMGR owns validation and all code-object transformations after the call. - Use ROCm/llvm-project#3008's explicit COMGR options API: ROCr calls `amd_comgr_hotswap_rewrite_with_options` with `AMD_COMGR_HOTSWAP_REWRITE_FLAG_ENTRY_TRAMPOLINES` when it requests entry trampolines, and keeps `amd_comgr_hotswap_rewrite` for legacy/default rewrites. - Keep `AMD_COMGR_HOTSWAP_ENTRY_TRAMPOLINES` as the ROCr policy knob: default enabled in ROCr, literal `0` disables, unset/empty/any other value enables the entry-trampoline request. - Cover concrete `gfx125*` targets and `gfx12-5-generic` while keeping non-gfx12.5 agents and source code objects unchanged. - Follow #7581 by keeping the entry-trampoline path keyed to the code-object ISA rather than using this path for processor retargeting. - Preserve the existing gfx1250 A0 source/target ISA pair and leave pass selection to COMGR. - Add unit and loader-path coverage for the local routing behavior without testing COMGR pass internals. ## Testing - `git diff --check` - `cmake --build build-rocrtst-hotswap-review --target hotswap_rewrite -j 8` - `ctest --test-dir build-rocrtst-hotswap-review -R hotswap --output-on-failure` - `cmake --build build-rocr-hotswap --target hsa-runtime64 -j 8`

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch 2 times, most recently from 3c42707 to c3065ae Compare June 22, 2026 17:31

lamb-j added the hotswap Related to the Comgr Hotswap feature label Jun 23, 2026

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch 11 times, most recently from e8897c6 to 280c69b Compare June 27, 2026 23:05

harsh-amd mentioned this pull request Jun 27, 2026

AMDGPU: add hotswap entry trampoline lit coverage #3098

Open

harsh-amd changed the title ~~[AMDGPU][COMGR] Add hotswap entry trampolines~~ AMDGPU: add hotswap entry trampoline core Jun 27, 2026

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch from 280c69b to 737534a Compare June 27, 2026 23:06

harsh-amd marked this pull request as ready for review June 28, 2026 17:50

harsh-amd requested review from chinmaydd and lamb-j as code owners June 28, 2026 17:50

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch from 737534a to 1de7a53 Compare June 28, 2026 18:09

chinmaydd reviewed Jun 30, 2026

View reviewed changes

Comment thread amd/comgr/src/hotswap/README.md Outdated

chinmaydd reviewed Jun 30, 2026

View reviewed changes

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch from 1de7a53 to 2ef5a67 Compare June 30, 2026 06:36

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch 2 times, most recently from 7eb0c18 to bcd7a3e Compare June 30, 2026 09:29

AMDGPU: add hotswap entry trampoline core

f357644

harsh-amd force-pushed the comgr-hotswap-entry-trampolines branch from bcd7a3e to f357644 Compare June 30, 2026 13:11

AMDGPU: expose hotswap rewrite options API

04ba3f5

AMDGPU: test hotswap rewrite options API

80cbb28

chinmaydd reviewed Jun 30, 2026

View reviewed changes

AMDGPU: document hotswap no-op behavior

b64f8f5

chinmaydd approved these changes Jun 30, 2026

View reviewed changes

harsh-amd mentioned this pull request Jun 30, 2026

Bump amd-llvm for hotswap entry trampolines ROCm/TheRock#6276

Open

chinmaydd merged commit 642c84c into ROCm:amd-staging Jun 30, 2026
52 of 57 checks passed

This was referenced Jun 30, 2026

[Comgr] Bump version to 3.4 for amd_comgr_hotswap_rewrite_with_options #3134

Merged

[Comgr] Document API versioning rule in AGENT_CONVENTIONS.md #3135

Merged

harsh-amd mentioned this pull request Jul 1, 2026

AMDGPU: reduce hotswap helper duplication #3138

Open

harsh-amd mentioned this pull request Jul 5, 2026

Bump gfx1250 hotswap integration submodules ROCm/TheRock#6346

Open

Uh oh!

Conversation

harsh-amd commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaydd commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harsh-amd commented Jun 30, 2026

Uh oh!

harsh-amd commented Jun 30, 2026

Uh oh!

chinmaydd commented Jun 30, 2026

Uh oh!

harsh-amd commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaydd Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaydd Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaydd commented Jun 30, 2026

Uh oh!

harsh-amd commented Jun 30, 2026

Uh oh!

chinmaydd commented Jun 30, 2026

Uh oh!

harsh-amd commented Jun 30, 2026

Uh oh!

Uh oh!

chinmaydd commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lamb-j commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

harsh-amd commented Jun 22, 2026 •

edited

Loading

chinmaydd commented Jun 30, 2026 •

edited

Loading

harsh-amd commented Jun 30, 2026 •

edited

Loading

chinmaydd Jun 30, 2026 •

edited

Loading

chinmaydd Jun 30, 2026 •

edited

Loading

chinmaydd commented Jun 30, 2026 •

edited

Loading